Skip to content

feat: Setting configuration with JSON string #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

feat: Setting configuration with JSON string #166

wants to merge 9 commits into from

Conversation

@sdelliot
Copy link
Collaborator

@sdelliot sdelliot commented Sep 10, 2025

This PR adds the ability for users to take in a properly formatted JSON string and set the FIREWHEEL configuration.

This JSON string will set/replace a subset of the configuration and any keys or sub-keys (or sub-sub-keys) not present will not be impacted or overwritten.
The string must include the top-level config key as well as any sub-keys needing modifications.

For example, to change the value for the config key logging.level from DEBUG to INFO, here is how that would not look:

Before Config

logging:
    cli_log: cli.log
    discovery_log: discovery.log
    firewheel_log: firewheel.log
    level: DEBUG
    minimega_log: minimega.log
    root_dir: /scratch/firewheel
    vmr_log_dir: vm_resource_logs
firewheel config set -j '{"logging":{"level":"INFO"}}'

After Config

logging:
    cli_log: cli.log
    discovery_log: discovery.log
    firewheel_log: firewheel.log
    level: INFO
    minimega_log: minimega.log
    root_dir: /scratch/firewheel
    vmr_log_dir: vm_resource_logs

@github-actions github-actions bot added documentation Improvements or additions to documentation feature New feature or request labels Sep 10, 2025
@mitchnegus
Copy link
Member

mitchnegus commented Sep 11, 2025 •
edited
Loading

I like that it will not require a user to have to pass the whole block to set one element. Do we need to consider whether there is ever a need to remove key-value pairs from the configuration? (In which case, this removes the method of just omitting it from the JSON as an option to accomplish that).

Maybe we just say that a FIREWHEEL config should have all the settings defined, and so we don't support removing a field. Not sure that's best, because I don't know how a user would say "go back to the default" (other than checking the docs and manually setting it to that value, which seems like an unfriendly thing to do).

If we do establish that expectation, it's probably worth being aware that the config-template.yml file seems to include all the options by default except system.firewheel_root_dir. (Doesn't break anything here, but it would be a violation of our new "rule" that every config file contains every option. Also interestingly we don't seem to use system.firewheel_root_dir anywhere in this repo.)

mitchnegus
mitchnegus reviewed Sep 11, 2025
View reviewed changes
@mitchnegus
Copy link
Member

mitchnegus commented Sep 11, 2025

Looks good to me other than the comments above.

To be "that guy", do we need/want a test or two?

@mitchnegus mitchnegus closed this Sep 11, 2025
@mitchnegus mitchnegus reopened this Sep 11, 2025
@gregjacobus
Copy link
Collaborator

gregjacobus commented Sep 11, 2025

I think it still makes sense to include this feature, but another option would be to use yq to modify the config file directly. This is probably what we should have done in #162 instead of that horrible sed command in hindsight. Downside is it requires installing something extra

@sdelliot sdelliot marked this pull request as draft September 11, 2025 12:00
@sdelliot
Copy link
Collaborator Author

sdelliot commented Sep 11, 2025

I think it still makes sense to include this feature, but another option would be to use yq to modify the config file directly. This is probably what we should have done in #162 instead of that horrible sed command in hindsight. Downside is it requires installing something extra

While I don't like adding another dependency, this would "only" be the case for the CI, and won't impact end users. Additionally, this would be a wholly optional thing for users. Another benefit to this approach is less code maintenance that we have to write, which is always a positive. On the other hand, yq looks challenging to understand/use. However, I suspect that we could get away with a brief example/reference in the config set documentation to point users in the right way.

I'm going to play with yq to see how easy it is to replace that sed command and leave this in draft until then.

@sdelliot sdelliot marked this pull request as ready for review October 6, 2025 16:10
@sdelliot
Copy link
Collaborator Author

sdelliot commented Oct 6, 2025

I think it still makes sense to include this feature, but another option would be to use yq to modify the config file directly. This is probably what we should have done in #162 instead of that horrible sed command in hindsight. Downside is it requires installing something extra

While I don't like adding another dependency, this would "only" be the case for the CI, and won't impact end users. Additionally, this would be a wholly optional thing for users. Another benefit to this approach is less code maintenance that we have to write, which is always a positive. On the other hand, yq looks challenging to understand/use. However, I suspect that we could get away with a brief example/reference in the config set documentation to point users in the right way.

I'm going to play with yq to see how easy it is to replace that sed command and leave this in draft until then.

Cycling back to this, I tried yq and it did look much better. However, in the end it seems like it may not be feasible to expect users to have yq if they want some automated post-install functionality (like configuring caches). Therefore, I still think this would be useful to have.

@sdelliot sdelliot requested a review from gregjacobus October 6, 2025 16:13
gregjacobus
gregjacobus requested changes Oct 6, 2025
View reviewed changes
Copy link
Collaborator

@gregjacobus gregjacobus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly sure why, but this doesn't allow spaces in the json string. For example, the following command fails saying it's invalid json, when it actually is valid:

firewheel config set -j '{"ansible":{"git_servers": []}}'

@mitchnegus
Copy link
Member

mitchnegus commented Oct 13, 2025 •
edited
Loading

Not exactly sure why, but this doesn't allow spaces in the json string. For example, the following command fails saying it's invalid json, when it actually is valid:

firewheel config set -j '{"ansible":{"git_servers": []}}'

Looks like the input arguments get split as strings on whitespace rather than as shell inputs, so quoted inputs are ignored and your string gets parsed as just '{"ansible":{"git_servers":.

@mitchnegus mitchnegus mentioned this pull request Oct 15, 2025
Merged
11 tasks
sdelliot pushed a commit that referenced this pull request Nov 6, 2025
@mitchnegus
fix: Fix config set (#207)
a05399c 
# Pull Request Title

## Description
The `firewheel config set -s ...` command does not properly handle
whitespace. It takes arguments from the command line and splits them
using whitespace (ignoring CLI indicators like quoted parameters).
Instead of using the `str.split` method, the `ConfigureFirewheel` object
(and the underlying `Config` object) should use `shlex.split`.

## Related Issue
See [related
discussion](#166 (review))
on #166.

## Type of Change
Please select the type of change your pull request introduces:
- [x] Bugfix
- [ ] New feature
- [ ] Documentation update
- [ ] Other (please describe):

## Checklist
- [x] This PR conforms to the process detailed in the [Contributing
Guide](https://sandialabs.github.io/firewheel/developer/contributing.html).
- [x] I have included no proprietary/sensitive information in my code. 
- [x] I have performed a self-review of my code.
- [ ] N/A ~I have commented my code, particularly in hard-to-understand
areas.~
- [ ] N/A ~I have made corresponding changes to the documentation.~
- [x] My changes generate no new warnings.
- [x] I have tested my code.
@sdelliot
Copy link
Collaborator Author

sdelliot commented Nov 11, 2025

Not exactly sure why, but this doesn't allow spaces in the json string. For example, the following command fails saying it's invalid json, when it actually is valid:

firewheel config set -j '{"ansible":{"git_servers": []}}'

Looks like the input arguments get split as strings on whitespace rather than as shell inputs, so quoted inputs are ignored and your string gets parsed as just '{"ansible":{"git_servers":.

Following up on this, this is resolved as of #207 and I have verified that spaces now work.

mitchnegus
mitchnegus requested changes Nov 11, 2025
View reviewed changes
@sdelliot
Copy link
Collaborator Author

sdelliot commented Nov 11, 2025

Looks good to me other than the comments above.

To be "that guy", do we need/want a test or two?

Always be "that guy"! I just added some test cases :)

@sdelliot sdelliot requested a review from mitchnegus November 11, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregjacobus gregjacobus Awaiting requested review from gregjacobus

@mitchnegus mitchnegus Awaiting requested review from mitchnegus

Requested changes must be addressed to merge this pull request.

Assignees

@mitchnegus mitchnegus

@gregjacobus gregjacobus

Labels

documentation Improvements or additions to documentation feature New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sdelliot @mitchnegus @gregjacobus